Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Removed timecop gem in favor of travel_to #351

Merged
merged 1 commit into from
Sep 29, 2023
Merged

Removed timecop gem in favor of travel_to #351

merged 1 commit into from
Sep 29, 2023

Conversation

seanpdoyle
Copy link
Contributor

With the release of rails 4.1, Timecop.freeze can be replaced with travel_to

Timecop.freeze 1.week.ago do
  # time sensitive code
end

travel_to 1.week.ago do
  # time sensitive code
end

Read more in the TimeHelpers module

inspired by http://brandonhilkert.com/blog/rails-4-1-travel-to-test-helper

@croaky
Copy link
Contributor

croaky commented May 23, 2014

Oh that's cool. This makes sense to me but have we done it on any apps yet?

@seanpdoyle
Copy link
Contributor Author

@croaky Also, I'm not sure how we've gone about communicating dependency removals in the past, but we'd probably want to point users to the docs for TimeHelpers (probably in the README or CHANGELOG).

I'd expect users of suspenders have grown accustom to timecop being bundled by default.

@calebhearth
Copy link
Contributor

Timecop does other things as well. If we need freeze for example, we still need the gem.

@seanpdoyle
Copy link
Contributor Author

@calebthompson freeze could be replaced by travel_to with a block couldn't it?

@calebhearth
Copy link
Contributor

Looks like, yeah. The only things in the README other than that are scale, which I can't see a general use case for, and safe_mode which I'd love to have be default.

Works for me, but I haven't used it either. As far as documenting the change, NEWS.md makes sense.

seanpdoyle
seanpdoyle reviewed May 23, 2014
View reviewed changes
NEWS.md Outdated
@@ -1,3 +1,12 @@
1.12.0 (May 23, 2014)

* Included [formulaic](https://github.com/thoughtbot/formulaic) for integration
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@calebthompson @croaky I've never written release notes before. Is this good enough?

Changes like this require a semver bump, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect.

@seanpdoyle
Copy link
Contributor Author

@croaky good to merge and bump the release? Should we ask for more opinions?

@croaky
Copy link
Contributor

croaky commented May 26, 2014

@seanpdoyle I'd like to hold off on merging until someone reports using this successfully on one of or more of our apps.

@seanpdoyle
Copy link
Contributor Author

Apparently the two behave differently.

Timecop.freeze sets Time.now to the given time and stops the flow of time within the block.
travel_to only sets Time.now to the given time. Time continues to flow like normal.

I'm not sure of how to reproduce the freeze behavior with travel_to

@croaky
Copy link
Contributor

croaky commented Jul 20, 2014

@seanpdoyle @arunagw @sgrif @sikachu @jferris How are we feeling about this PR? Can Timecop definitely be replaced by travel_to?

@calebhearth
Copy link
Contributor

Just ran into a scenario where travel_to wouldn't work:

Google's oauth provides an access token which expires. As part of the refresh process, google sends an amount of time for which the new token will be valid in the form of a number of seconds.

To verify that the expiration time is properly set to now + number_of_seconds, I need to freeze time as well as travel to a known base time.

@croaky
Copy link
Contributor

croaky commented Jul 22, 2014

It seems like for now, we should probably keep Timecop in Suspenders. I think now might be a good time to close the PR. If others feel differently, please feel free to re-open.

@croaky croaky closed this Jul 22, 2014
@sgrif
Copy link
Contributor

sgrif commented Jul 23, 2014

Is the concern whether travel_to must work for all cases, or whether it works for the general case? Apps can always add timecop if they need it. clearance also doesn't work for all cases, but it's our default -- I have no strong feelings on the subject, as I personally avoid both timecop and travel_to, but just interested by the motivations.

@croaky
Copy link
Contributor

croaky commented Jul 23, 2014

The gems we have in Suspenders should be gems we think we need on the majority of new apps. Clearance is our default choice for authentication but is not in Suspenders.

If we don't think we need time freezing by default in projects, lets remove Timecop. I thought we did feel we need it, though.

What do you do instead, @sgrif, if you're avoiding both Timecop and travel_to?

@sgrif
Copy link
Contributor

sgrif commented Jul 23, 2014

Dependency injection. :)

@derekprior derekprior deleted the sd-remove-timecop branch January 7, 2015 22:23
@mike-burns
Copy link
Contributor

How have we been testing OAuth lately? Have we been using timecop in these tests, or some other form of stubbing? It might be worth revisiting this now.

@jmeinerz
Copy link

@mike-burns this may indeed be a good time to revisit this. In fact freeze_time was added to ActiveSupport in Rails v5.2

@composerinteralia composerinteralia restored the sd-remove-timecop branch April 6, 2020 22:37
Base automatically changed from master to main March 5, 2021 19:05
@thiagoa
Copy link
Contributor

thiagoa commented Aug 26, 2021

It seems most issues reported here have been solved by the Rails team.

  1. freeze_time has been implemented in TimeHelpers
  2. safe mode is no longer needed (i.e. enforce the use of a block when freezing the time) because time stubs are automatically cleared on teardown
  3. Nesting travel methods is still not possible. However, I don't think we should necessarily support that use case. Consciously opting for Timecop is always a choice in these rare cases.

croaky
croaky approved these changes Sep 29, 2023
View reviewed changes
Copy link
Contributor

@croaky croaky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot's changed in 9 years :) Go for it!

@seanpdoyle seanpdoyle changed the base branch from main to rake-standard-fix September 29, 2023 16:04
Base automatically changed from rake-standard-fix to main September 29, 2023 16:43
@seanpdoyle
Copy link
Contributor Author

It is time.

@seanpdoyle
Removed timecop in favor of travel_to
830b9ec 
With the release of `rails` 4.1, `Timecop.freeze` can be replaced with
`travel_to`

```ruby
Timecop.freeze 1.week.ago do
  # time sensitive code
end

travel_to 1.week.ago do
  # time sensitive code
end
```

Read more in the
[TimeHelpers](https://github.com/rails/rails/blob/a6f55fe257512731d7f3f41976648d99e9ec95be/activesupport/lib/active_support/testing/time_helpers.rb#L43)
module.
@seanpdoyle seanpdoyle merged commit 55cb5c2 into main Sep 29, 2023
1 check passed
@seanpdoyle seanpdoyle deleted the sd-remove-timecop branch September 29, 2023 17:01
@stevepolitodesign
Copy link
Contributor

Noting we still have a reference to Timecop here:

https://github.com/thoughtbot/suspenders/blob/main/templates/spec_helper.rb

However, this will be removed in https://github.com/thoughtbot/suspenders/tree/suspenders-3-0-0-testing-generator

@seanpdoyle
Copy link
Contributor Author

Thank you for catching that @stevepolitodesign! Are you interested in opening a PR to remove that line in the short term?

If not, I'm happy to open one.

stevepolitodesign added a commit that referenced this pull request Jan 12, 2024
@stevepolitodesign
Remove reference to Timecop from spec_helper.rb
05f05ac 
Follow-up to #351

Now that `Timecop` has been removed, we no longer need to configure
RSpec to use it.
@stevepolitodesign stevepolitodesign mentioned this pull request Jan 12, 2024
Closed
@stevepolitodesign
Copy link
Contributor

@seanpdoyle I've opened #1155 to address this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@croaky croaky croaky approved these changes

@stevepolitodesign stevepolitodesign Awaiting requested review from stevepolitodesign stevepolitodesign is a code owner

@thiagoa thiagoa Awaiting requested review from thiagoa

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@seanpdoyle @croaky @calebhearth @sgrif @mike-burns @jmeinerz @thiagoa @stevepolitodesign @composerinteralia